Skip to content

api: Add unreadMsgs in initial snapshot #266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

chrisbobbe
Copy link
Collaborator

@chrisbobbe chrisbobbe commented Aug 11, 2023

Our own data structure in PerAccountStore will be constructed from this value, and it will probably have quite a different shape; for example, we're likely to want to collapse the distinction between 1:1 and group DMs.

But now we're validating this part of the /register response and making it available to feed into those new internal data structures.

Related: #253

@chrisbobbe chrisbobbe added the a-api Implementing specific parts of the Zulip server API label Aug 11, 2023
@chrisbobbe chrisbobbe requested a review from gnprice August 11, 2023 23:41
@chrisbobbe chrisbobbe marked this pull request as draft August 11, 2023 23:57
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Generally looks good; couple of small comments below, and there's those two chat threads.

final String topic;
final int streamId;
final List<int> unreadMessageIds;
// final List<int> senderIds; // deprecated; ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// final List<int> senderIds; // deprecated; ignore

This one isn't merely deprecated but (at least according to the docs) actually absent for current servers, so in particular it's only documented within a "Changes" note:
image

Because it's not in the main line of the documentation, we don't need to specifically mention that we've looked at it and consciously chosen not to put it in the type.

Comment on lines 140 to 143
/// An item in `unread_msgs.pms`.
///
/// For docs, search for "unread_msgs:" and see "pms:"
/// in <https://zulip.com/api/register-queue>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
/// An item in `unread_msgs.pms`.
///
/// For docs, search for "unread_msgs:" and see "pms:"
/// in <https://zulip.com/api/register-queue>.
/// An item in [UnreadMessagesSnapshot.dms].

(and similarly the other two below)

I think once you search for "unread_msgs:" and are looking at that section, it's easy to find the individual subsections for these pieces of it. Also if you're looking at any one of them it's quite likely you're looking at other parts of unread_msgs anyway.

Conversely it's nice to draw the links between these types explicitly, and to keep them each compact the better to have several of them on screen at the same time.

@chrisbobbe chrisbobbe force-pushed the pr-unread-msgs-snapshot branch from 553f9a8 to bc6e72a Compare August 16, 2023 16:45
@chrisbobbe
Copy link
Collaborator Author

Thanks! Revision pushed.

@chrisbobbe chrisbobbe changed the title wip api: Add unreadMsgs in initial snapshot api: Add unreadMsgs in initial snapshot Aug 16, 2023
@chrisbobbe chrisbobbe marked this pull request as ready for review August 16, 2023 16:46
Our own data structure in PerAccountStore will be constructed from
this value, and it will probably have quite a different shape; for
example, there, we're likely to want to collapse the distinction
between 1:1 and group DMs.

But now we're validating this part of the /register response and
making it available to feed into those new internal data structures.

Related: zulip#253
@gnprice
Copy link
Member

gnprice commented Aug 17, 2023

Thanks! Looks good; merging.

@gnprice gnprice force-pushed the pr-unread-msgs-snapshot branch from bc6e72a to e7073c2 Compare August 17, 2023 03:27
@gnprice gnprice merged commit e7073c2 into zulip:main Aug 17, 2023
@chrisbobbe chrisbobbe deleted the pr-unread-msgs-snapshot branch August 24, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-api Implementing specific parts of the Zulip server API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants